Conversation
idanpopovich8
left a comment
idanpopovich8
left a comment
There was a problem hiding this comment.
hi i left you some code reviews inside
GOOD JOB!!!!!
| </style> | ||
| </head> | ||
| <body> | ||
| .attribution { |
There was a problem hiding this comment.
For next time, we usually writing our design in a separate file called style.css
| } | ||
|
|
||
| .attribution a { | ||
| color: hsl(228, 45%, 44%); |
There was a problem hiding this comment.
I recommend using CSS variables — they’re easier to work with and are generally considered best practice.
| width: 40vw; | ||
| height: 50vh; | ||
| border-radius: 12px; | ||
| padding: 25px 5px; |
| .field { | ||
| display: flex; | ||
| flex-direction: row; | ||
| width:250px; |
There was a problem hiding this comment.
Coniser that its not It’s not recommended — and even considered incorrect — to use fixed pixel sizes for a div. If the user changes their screen size, the entire layout can break
| justify-content: center; | ||
| align-items: center; | ||
| margin-top: 20px; | ||
| width:250px; |
There was a problem hiding this comment.
Same comment as before about pixels, try to use % or Vh and Vw
| <div class="title"> Stay updated!</div> | ||
| <div class="sub-title"> Join 60,000+ product managers receiving monthly updates on:</div> | ||
| <div class="line-1"> | ||
| <img src=assets\images\icon-list.svg> |
| <div class="email-title"> | ||
| Email address | ||
| </div> | ||
| <div class=field> |
There was a problem hiding this comment.
In here you have to classes named "field" consider it is not good practice try to be more creative next time LOL : )
| <div class="white-ground"> | ||
| <div class="text"> | ||
| <div class="title"> Stay updated!</div> | ||
| <div class="sub-title"> Join 60,000+ product managers receiving monthly updates on:</div> |
There was a problem hiding this comment.
In my opinion you have to many divs. in here the "sub-title" div supposed to be in a h1 label it will make your code easier to read
| <body> | ||
|
|
||
| <div class="page"> | ||
| <div class="grey-ground"> |
There was a problem hiding this comment.
Try to use more logical names. grey-ground class its most commonly about colors and not about roles in my page.
maybe page-background will be better for next time.
2 participants
test